🦋 Changeset detected

Latest commit: ea22839

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@mitchellhamilton I fixed the spelling issue and only added the "worker" condition to packages that need it. I was sure that the spelling issue was the reason that the extra fields were not being generated. However, I have run preconstruct fix and precontruct build` but the worker conditions are still not being generated.

I want to be respectful of your time so I'll keep trying to debug on my end. Thanks again for all your help with this!

However, I have run preconstruct fix and precontruct build` but the worker conditions are still not being generated.

You might not be running preconstruct from the root of the repo? (As in, you need to run it from the root of the repo)

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit ea22839:

@mitchellhamilton I deleted node_modules and then re-installed and it seems to be working now. Thanks again for all your help!

Looks great 😃. Question: have you only generated exports for specific packages? It seems like we should do this for all of our packages.

It seems like we should do this for all of our packages.

I'm open to making this change if others agree that it would be useful. The reason I didn't initially include it is because it doesn't not actually change anything. Preconstruct builds (and therefore emotion builds) specifically target bundlers and most bundlers know how to read from the main, module and browser fields of package.json so when using only those conditions, it doesn't not add any value to also include the exports field. I only added the exports field where there was a new condition that we wanted to use and exports is the only way to use it (worker).

I imagine that someday, preconstruct will be extended to support Node ESM (ie the imports condition) and then it would be valuable to add the exports fields to all packages.

Your first paragraph makes sense to me! I'm not 100% sure what you mean by:

I imagine that someday, preconstruct will be extended to support Node ESM (ie the imports condition)

I tested your changes and came up with this patch:

diff --git a/packages/react/package.json b/packages/react/package.json
index de9470c1..7dcf755f 100644
--- a/packages/react/package.json
+++ b/packages/react/package.json
@@ -8,7 +8,8 @@
   },
   "exports": {
     ".": {
-      "module": {
+      "types": "./types/index.d.ts",
+      "import": {
         "worker": "./dist/emotion-react.worker.esm.js",
         "browser": "./dist/emotion-react.browser.esm.js",
         "default": "./dist/emotion-react.esm.js"
@@ -16,7 +17,8 @@
       "default": "./dist/emotion-react.cjs.js"
     },
     "./jsx-runtime": {
-      "module": {
+      "types": "./types/jsx-runtime.d.ts",
+      "import": {
         "worker": "./jsx-runtime/dist/emotion-react-jsx-runtime.worker.esm.js",
         "browser": "./jsx-runtime/dist/emotion-react-jsx-runtime.browser.esm.js",
         "default": "./jsx-runtime/dist/emotion-react-jsx-runtime.esm.js"
@@ -24,7 +26,7 @@
       "default": "./jsx-runtime/dist/emotion-react-jsx-runtime.cjs.js"
     },
     "./_isolated-hnrs": {
-      "module": {
+      "import": {
         "worker": "./_isolated-hnrs/dist/emotion-react-_isolated-hnrs.worker.esm.js",
         "browser": "./_isolated-hnrs/dist/emotion-react-_isolated-hnrs.browser.esm.js",
         "default": "./_isolated-hnrs/dist/emotion-react-_isolated-hnrs.esm.js"
@@ -32,13 +34,15 @@
       "default": "./_isolated-hnrs/dist/emotion-react-_isolated-hnrs.cjs.js"
     },
     "./jsx-dev-runtime": {
-      "module": {
+      "types": "./types/jsx-dev-runtime.d.ts",
+      "import": {
         "worker": "./jsx-dev-runtime/dist/emotion-react-jsx-dev-runtime.worker.esm.js",
         "browser": "./jsx-dev-runtime/dist/emotion-react-jsx-dev-runtime.browser.esm.js",
         "default": "./jsx-dev-runtime/dist/emotion-react-jsx-dev-runtime.esm.js"
       },
       "default": "./jsx-dev-runtime/dist/emotion-react-jsx-dev-runtime.cjs.js"
     },
+    "./macro": "./macro.js",
     "./package.json": "./package.json"
   },
   "types": "types/index.d.ts",

Description of changes:

• The "types" key was necessary for TypeScript to find the type declarations — I'm testing with "moduleResolution": "nodenext".
• Changed "module" condition to "import". I reviewed the relevant Node.js documentation and did not see "module" listed as a valid condition. Maybe I'm missing something here?
• Added missing export for @emotion/react/macro.

Assuming what I did is correct, these same changes will need to be made to the other package.jsons you modified too.

@srmagura preconstruct (and therefore emotion) purposely does not support the node specific imports condition inside the exports field. We instead use a similar condition module which is used by bundlers like webpack, rollup, ect... See you can see more of a discussion about this here: preconstruct/preconstruct#435 (comment)

I'm not a maintainer of emotion so I can not speak for the group but my understanding is that emotion will likely not support ESM node (ie the imports condition) until it is supported in preconstruct. This also means that they will likely not support moduleResolution: "nodenext" until then as well.

I'm not a maintainer of emotion so I can not speak for the group but my understanding is that emotion will likely not support ESM node (ie the imports condition) until it is supported in preconstruct. This also means that they will likely not support moduleResolution: "nodenext" until then as well.

We should make it possible for emotion to be imported by Node ESM (including with TypeScript's "moduleResolution": "nodenext") but we definitely won't be providing Node ESM any time soon (because shipping Node ESM isn't really beneficial for consumers)

@mitchellhamilton what would that look like? Do you feel like that should be included with this PR? Should I manually add the “imports” and “types” conditions to all packages?

Do you feel like that should be included with this PR?

Yes - not doing so would be a regression from the current behavior

Should I manually add the “imports” and “types” conditions to all packages?

We should not use the import condition. Using it is not necessary whatsoever for importing into Node ESM.

Rather than adding the types condition (because otherwise you'd have to duplicate all of the conditions because preconstruct.extra will replace not merge conditions), I'd say we should move the .d.ts files to be next to the appropriate source .js files and Preconstruct will do the right things with them (i just made this work in Preconstruct - https://github.com/preconstruct/preconstruct/releases/tag/%40preconstruct%2Fcli%402.2.0)

@Andarist do you have any thoughts on the breaking change-ness of adding the exports field for emotion? Should we just not care if people are importing stuff from dist? Should we keep the files in /types and add them to the exports field (probably at least will need to for @emotion/react/types/css-prop)? What were you thinking for the TypeScript migration in regards to the types directories?

I'm not an expert on this stuff but here's my 2 cents:

do you have any thoughts on the breaking change-ness of adding the exports field for emotion? Should we just not care if people are importing stuff from dist?

It seems fine to do this as a minor version bump. AFAIK we've never documented importing stuff directly from dist so it's not part of our "public API".

Should we keep the files in /types and add them to the exports field (probably at least will need to for @emotion/react/types/css-prop)? What were you thinking for the TypeScript migration in regards to the types directories?

I vote for killing the /types directories, with an exception for @emotion/react/types/css-prop and any other .d.ts files people may be referencing explicitly.

The /types directories still exist in the ts-migration branch, which feels weird to me since it's usually just

export * from '..';

Rather than adding the types condition (because otherwise you'd have to duplicate all of the conditions because preconstruct.extra will replace not merge conditions), I'd say we should move the .d.ts files to be next to the appropriate source .js files and Preconstruct will do the right things with them (i just made this work in Preconstruct - https://github.com/preconstruct/preconstruct/releases/tag/%40preconstruct%2Fcli%402.2.0)

This might be hard to pull off here because dtslint setup requires those types to live in its own directory. Unless we reexport what is in /types from within /src

@Andarist do you have any thoughts on the breaking change-ness of adding the exports field for emotion? Should we just not care if people are importing stuff from dist? Should we keep the files in /types and add them to the exports field (probably at least will need to for @emotion/react/types/css-prop)? What were you thinking for the TypeScript migration in regards to the types directories?

I think that all of that should be OK to do. We never made any guarantees about the internal file structure. So we only need to support what has been documented etc.

The /types directories still exist in the ts-migration branch, which feels weird to me since it's usually just export * from '..';

Yeah, this was just to make dtslint understand things. Maybe it's not needed with the updated version of dtslint? I think it might be hard to kill /types before we complete the ts-migration work.

Unless we reexport what is in /types from within /src

This is what I was thinking we would do - though probably not include them in the exports field

You might need to use .mjs extensions for esm for better compatibility, see https://github.com/sheremet-va/dual-packaging

I've pushed some changes to this branch. I think that the only leftover to fix is the types issue. I've upgraded the Preconstruct already but didn't yet have time to add those reexporting src/*.d.ts files and play around with it. Gonna take a look at this later.

Are there any other blockers that come to your mind @mitchellhamilton ?

@mitchellhamilton @Andarist FWIW, I have tested this version of emotion with Cloudflare workers specifically and it works with no issues!

I think we may be ready to merge! 🎉

Yep, I'm a little bit scared of this PR but I'll be ripping this bandaid off soon.